Read a realm identifier without assuming it is a URL - #5931
Conversation
Four sites take a realm identifier and immediately ask a URL question of it. Each is safe today only because the realm list happens to hold URL forms, and each fails differently the moment an entry is a registered prefix. `routes/index.gts` matched a card path against a realm path through `new URL(realm).pathname`, which throws for a prefix. The realm picker derived its label from the last path segment inside a `try`, so a prefix answered "Unknown Workspace" rather than the realm's name — a wrong label, no error. Both now take the segments of whichever form they are handed, through one helper that says why parsing loses the prefix case. `getRealmServersForRealms` skipped realms served by the test realm's origin. A prefix has no origin, so the identifier resolves through the VirtualNetwork before the comparison instead of being parsed as though it were already a URL. No behaviour changes: every identifier these see is URL-form today, and the `Integration | Store` module reads 81 passed against main's 81. This is the form-agnostic groundwork on its own, separated from the canonical-seed change that would depend on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Preview deploymentsHost Test Results 1 files ± 0 1 suites ±0 2h 8m 18s ⏱️ + 4m 13s Results for commit 1ccbb98. ± Comparison against earlier commit bd52016. Realm Server Test Results 1 files ± 0 1 suites ±0 22m 27s ⏱️ + 4m 19s Results for commit 1ccbb98. ± Comparison against earlier commit bd52016. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 340256853a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .filter((part) => part !== ''); | ||
| // A realm identifier may be a URL or a registered prefix, and only | ||
| // the first has a pathname; take the segments of whichever it is. | ||
| let realmPathParts = realmIdentifierSegments(realmUrl); |
There was a problem hiding this comment.
When hostsOwnAssets is enabled and this list starts containing a registered prefix, these segments do not represent the realm's mounted URL path. For example, @cardstack/base/ produces ['@cardstack', 'base'], while its mapped URL is mounted at /base/, so a normal base/foo route does not match and is resolved against the default realm instead. If the card path does contain the prefix segments, the later new URL(..., realmUrl) receives the non-URL prefix and throws. Resolve the identifier through virtualNetwork.toURL() before extracting the pathname and reuse that resolved URL as the base.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Correct on both counts, and the first one is the more interesting: my helper was answering the wrong question here. Fixed in bd52016.
Verified rather than assumed:
@cardstack/base/ segments -> ["@cardstack", "base"]
https://x/base/ pathname -> ["base"]
new URL('/base/foo', '@cardstack/base/') -> TypeError: Invalid URL
So a base/foo route would have matched nothing and fallen through to the default realm, and the base argument throws — silent wrong-realm resolution in one direction, a crash in the other.
The underlying mistake was conflating two questions behind one helper. What is this realm called and where is this realm mounted have the same answer for a URL identifier and different answers for a prefix, so a single "segments" function reads as correct right up until the forms diverge. This route asks the second question, so it now resolves each identifier through virtualNetwork.toURL() once and both matches on the resulting pathname and reuses it as the base — which is your suggestion exactly.
The helper stays for the realm picker, which genuinely is naming rather than locating: both forms end in the segment that names the realm, so a label built from it is right either way. Its doc now scopes it to that and points anything matching a request path at the VirtualNetwork, so the next caller does not repeat the conflation.
One consequence worth noting: with the resolution in place the site no longer needs its no-url-from-realm-identifier suppression, so that came out too — --report-unused-disable-directives would have caught it regardless, but it is a small sign the change is the right shape.
Locally against an environment-mode stack: Integration | Store 81/0 and Acceptance | basic tests 4/0, matching main.
Deriving segments from the identifier answers the wrong question here. The route asks which realm *serves* a card path, and a registered prefix does not carry that: `@cardstack/base/` names two namespace segments while the realm it maps to is mounted at `/base/`, so a `base/foo` route would match nothing and fall through to the default realm. The base argument below has the same requirement — a prefix is not a valid base URL. Resolving each identifier once fixes both: the match compares mounted paths, and the resolved URL is what `new URL` is handed. The segment helper stays for the realm picker, which is naming a realm rather than locating one — both forms end in the segment that names it. Its doc now says that is the whole of its remit, and points anything matching a request path at the VirtualNetwork instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Read the three call sites against the identifier forms the VirtualNetwork actually produces, then swept the edited files for the same question asked somewhere the lint rule cannot see it. Did not audit the wider availableRealmIdentifiers consumer set.
No regressions: both behavioural changes hold for every form these sites can see today. Two asks before merge — the picker's selection path throws for exactly the input this PR makes its label path handle, and nothing executable pins any of the new behaviour.
Recommendations:
- Resolve the realm-picker's option ids so the selection path survives a prefix — see the file comment on
realm-picker/index.gts. This is the one I'd hold for. - Pin the prefix branch with a test. Every identifier these sites see is URL-form, so matching test counts against main is consistent with the new branches never executing — no suite would notice if a later refactor reverted them.
realmIdentifierSegmentsis pure and takes four assertions;addRealmMapping/removeRealmMappingalready scope a temporary prefix to a single test (tests/integration/realm-test.gtsregisters one this way), which is enough to drivegetRealmServersForRealmswith a prefix identifier. - Resolve or document the fallback base in
routes/index.gts— inline comment on the??line. - The token lookup in
getRealmServersForRealmsis still spelling-exact — inline comment there.
Adjacent, out of scope: vn.isRegisteredPrefix(x) ? vn.toURL(x).href : x now has six homes across store.ts, realm.ts, and the two sites here. Since every registered realm-mapping key is @-scoped, toURL alone already has these semantics — worth one home on VirtualNetwork before it drifts, and worth an assertion in addRealmMapping if the @-scoped shape is meant to be an invariant rather than a habit.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] onChange still does new URL(opt.id), and opt.id is a realm identifier straight from availableRealmIdentifiers — so this component's hard failure is untouched while its soft one is fixed. A prefix reaching the label path now yields the realm's name; the same prefix reaching a click throws TypeError: Invalid URL and takes the picker's selection with it.
The lint rule can't reach it: PickerOption.id is plain string, so the RealmIdentifier brand is gone the moment the identifier is stored into an option and read back. That makes green lint no evidence for this file.
Resolving at the parse point isn't enough either, because the two ends mint ids in different spellings: realmOptions uses the identifier, pickerSelected uses url.href, and Picker matches selection by o.id === option.id (packages/boxel-ui/src/components/picker/index.gts). With a prefix registered, a selected realm would come back as an option that never renders selected.
Both fall out if the ids are minted resolved — inject the network service and build realmOptions ids as vn.toURL(identifier).href. Then every opt.id is URL-form, new URL(opt.id) is safe, and the option/selected spellings agree.
Class: pre-existing, not introduced here — but it is in this file and inside the PR's own stated scope, and it is the failure that throws. Non-blocking only if you'd rather split it; I'd take it here.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Taken here. realmOptions now mints ids through resolvedRealmURLHref, so every opt.id is URL-form: onChange's parse is safe, and the option/selected spellings agree so Picker's id comparison can't miss. Label and realm.info() still read the identifier as given — info() is form-agnostic already, and realmIdentifierSegments wants the identifier, not the mount path.
Confirmed no consumer depends on the old spelling: onChange hands out URL[], and selectedURLs is minted by the parent rather than from option ids.
| }); | ||
| cardUrl = new URL( | ||
| `/${cardPath}`, | ||
| realmUrl ?? this.realm.defaultReadableRealm.path, |
There was a problem hiding this comment.
[Claude Code 🤖] The ?? alternative is not resolved. defaultReadableRealm.path is a realm identifier too — a key of realm.realms, else ENV.resolvedBaseRealmURL — and realm.realms is keyed by whatever spelling created each resource (getOrCreateRealmResource sets the key verbatim; knownRealm goes to unresolveURL precisely because those keys are not guaranteed URL-form). So the comment above claiming the base is the resolved URL holds for the realmUrl half only.
The rule is silent here because defaultReadableRealm is typed { path: string; info: RealmInfo } — unbranded — so nothing will flag this if the assumption stops holding. The suppression this hunk deletes was the only record that this half was unverified.
Either run it through vn.toURL() as well, or state in the comment what makes the default realm exempt.
Class: pre-existing assumption, kept load-bearing by this hunk. Non-blocking.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Resolved rather than documented — defaultReadableRealm.path goes through the same resolvedRealmURLHref as the entries above it. Your reading is right that it's a realm.realms key and so not guaranteed URL-form, and being typed as a plain string is exactly why nothing would flag it later.
| continue; | ||
| } | ||
| } | ||
| let token = sessionTokens[normalizedRealmURL] ?? sessionTokens[realmURL]; |
There was a problem hiding this comment.
[Claude Code 🤖] Two lines below the resolve, the token lookup is still exact-string on the unresolved spellings. A prefix identifier matches neither key — session tokens are persisted under the realm resource's own url — so it continues, and when it is the only realm in realms the function drops through to realmServerURLs.add(this.normalizeRealmServerURL(this.url.href)) and answers with the own realm server rather than the realm's. Same silent-wrong-answer shape the origin comparison had, one branch further down.
Worth resolving once at the top of the loop and using that as an additional lookup key, so the function is form-agnostic end to end rather than only at the skip. If session tokens are in fact always URL-keyed, that invariant is the thing to say here.
Class: pre-existing. Non-blocking, and fine as a follow-up — but the function reads as prefix-safe after this change, which is what makes it worth naming.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed rather than deferred. The resolve moved to the top of the loop and its result is now both the origin comparison's input and an additional token key, so the function is form-agnostic end to end.
I kept all three keys rather than assuming tokens are URL-keyed: a resource files its token under whatever spelling created it, so a prefix-created resource files under the prefix. The integration test covers both directions.
The picker read a realm identifier without assuming it was a URL on its label path, while its selection path still parsed one. `PickerOption.id` is a plain string, so the identifier's brand is gone the moment it is stored into an option — nothing downstream could see that a prefix had arrived, and no lint rule reaches it. Two failures followed from that. `onChange` parsed `opt.id` back into a `URL`, which throws for a prefix and takes the picker's selection with it. And the two ends minted ids in different spellings — options from the identifier, selected from `URL.href` — while `Picker` decides which option is selected by comparing ids, so a selected realm would come back as an option that never rendered selected. Minting option ids resolved settles both: every `opt.id` is URL-form, and the two ends agree. `resolvedRealmURLHref` gives the three call sites that need this one home. It is not the same as `virtualNetwork.toURL`, which covers both spellings but throws on an identifier it cannot place; these are display and lookup paths that must not fail a render over an unplaceable realm.
The route's fallback base is a realm identifier like the entries it falls back from: `defaultReadableRealm.path` is a key of `realm.realms`, which is keyed by whatever spelling created each resource. It is typed as a plain string, so nothing would flag it if that assumption stopped holding. In `getRealmServersForRealms` the token lookup was spelling-exact, two lines below a resolve that made the origin comparison form-agnostic. A prefix matches neither key, so the realm is skipped; when it is the only realm, the empty result set makes the function answer with this realm server rather than the realm's — a wrong answer returned quietly. Resolving once at the top of the loop lets both questions ask in URL form.
Every identifier these paths see today is URL-form, so the prefix branches never execute and no suite would notice a refactor that dropped them. `realmIdentifierSegments` and `resolvedRealmURLHref` are pure, so both their branches are asserted directly, including that the latter returns an unplaceable identifier untouched where `toURL` throws. For `getRealmServersForRealms`, a scoped realm mapping drives the function with a prefix identifier, with the token filed under each spelling in turn. The mapped realm is given an origin of its own because the function skips anything resolving to the test realm's origin, which would otherwise hide the lookup under test.
|
[Claude Code 🤖] All four addressed and pushed; the picker one is fixed here rather than split. On the adjacent point — |
2 participants
Claude: Four sites take a realm identifier and immediately ask a URL question of it. Each is safe today only because the realm list happens to hold URL forms, and each fails differently the moment an entry is a registered prefix.
routes/index.gtsmatched a card path against a realm path throughnew URL(realm).pathname, which throws for a prefix. The realm picker derived its label from the last path segment inside atry, so a prefix answered "Unknown Workspace" rather than the realm's name — a wrong label, no error. Both now take the segments of whichever form they are handed, through one helper that says why parsing loses the prefix case.getRealmServersForRealmsskipped realms served by the test realm's origin. A prefix has no origin, so the identifier resolves through the VirtualNetwork before the comparison instead of being parsed as though it were already a URL.No behaviour changes: every identifier these see is URL-form today, and the
Integration | Storemodule reads 81 passed against main's 81. This is the form-agnostic groundwork on its own, separated from the canonical-seed change that would depend on it.